refactor(v4): shared BaseEstimator mixin - transactional set_params library-wide (2(c)-i) - #741
Merged
Merged
Conversation
… transactional probe re-init set_params library-wide (2(c)-i)
diff_diff/_base.py replaces the 25 hand-rolled get_params/set_params pairs
(the exhaustive inventory found 25 defining classes, correcting v4-design's
count of 24; changes_in_changes.py holds two) with one leaf-module mixin:
- get_params introspects the __init__ signature (per-class cls.__dict__
cache; KEYWORD_ONLY accepted for SpilloverDiD's all-keyword signature;
VAR_POSITIONAL/VAR_KEYWORD fail loudly) and uniformly accepts deep= -
previously 4/25 - so every estimator's surface is signature-compatible
with sklearn.base.clone and can never drift from its constructor.
- set_params is TRANSACTIONAL via probe re-init: strict unknown-key gate
("Unknown parameter: {key}" - method names and private attributes now
raise everywhere, replacing 17 permissive hasattr gates and one silent
no-op), then an overridable _normalize_set_params hook, then
type(self)(**merged) which raises before self is touched, then adoption
of parameter attrs plus declared derived-config attrs from the probe.
Validation is exactly __init__'s validation, eagerly and atomically, on
all 25 classes (previously 6 divergent shapes, 12 non-transactional).
- Per-class accommodations are declarative: _PARAM_ATTR_ALIASES
(DifferenceInDifferences raw vcov_type at _vcov_type_arg, PreTrendsPower
power->target_power), _DERIVED_CONFIG_ATTRS (DiD vcov-resolution trio;
_vcov_type_explicit on CallawaySantAnna/SunAbraham/WooldridgeDiD), and
DiD's 4-line _normalize_set_params override reproducing the pinned
robust=-alone re-derivation. SyntheticDiD's __init__ now stores its raw
conley_metric/conley_kernel (None in any legal construction) and
normalizes the deprecated lambda_reg/zeta to None, so the probe emits
the pinned DeprecationWarning exactly once per call that passes them and
get_params mirrors the full signature (the old hand-rolled dict omitted
lambda_reg/zeta and hardcoded six None literals).
Behavior changes (all fail-loudly; CHANGELOG has the user-facing list):
ten estimators whose set_params accepted constructor-rejected values until
fit() - CallawaySantAnna, TripleDifference, TwoStageDiD, ImputationDiD,
SunAbraham, StackedDiD, StaggeredTripleDifference, SpilloverDiD, TROP,
PreTrendsPower - now raise at set_params (their eight executable
lazy-validation test pins flipped; fit-time re-checks stay as a second
layer against direct attribute mutation, comments rewritten accordingly);
BaconDecomposition rejects unknown keys; HonestDiD gains rollback;
SpilloverDiD gains batch atomicity; TROP's set_params(lambda_*_grid=[])
now resolves to the constructor's default grid; ContinuousDiD.dvals /
PreTrendsPower.violation_weights list updates store the
constructor-normalized ndarray; get_params key order follows __init__.
Configured DCDH clone-identity failure (pre-existing paths_of_interest
canonicalization) and the sklearn-out-of-dev-deps posture are recorded in
DEFERRED.md; the never-had-the-pair classes are a TODO.md follow-up.
tests/test_base_estimator.py pins the contract on a DYNAMICALLY discovered
roster (every __all__ class with fit must mix BaseEstimator in;
LinearRegression is the one reasoned exclusion): signature<->get_params
sync, re-instantiation round-trip, strict-gate + batch atomicity,
value-level rollback (per-class bad-value table), deep= triple-equality,
post-normalization set_params==re-init equivalence, fitted-state
preservation, and sklearn clone under importorskip. premerge_scan's
Check C now fires only for hand-rolled in-body get_params (a base-name
whitelist cannot see SyntheticDiD's indirect inheritance; the contract
suite is the stronger replacement), with fixtures both ways.
Docs: CLAUDE.md inheritance map rewritten around the mixin;
CONTRIBUTING.md new-parameter checklist reworked (TWFE/MPD inherit
automatically; SyntheticDiD must mirror or reject explicitly); REGISTRY's
EfficientDiD eager-validation deviation Note retired in favor of the
library-wide contract; the two paper reviews echoing the old map updated;
ten docs/api pages gain :inherited-members: so the relocated methods stay
rendered; v4-design section 7/9 marked shipped with the corrected count.
Overall assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive summary
Methodology
Code QualityNo findings. The shared implementation rejects unknown keys before mutation and adopts validated probe state transactionally ( PerformanceNo findings. Probe construction adds minor configuration-time work but does not affect fitting or inference performance. MaintainabilityNo findings. Constructor introspection, parameter aliases, and derived-state declarations provide a coherent extension mechanism ( Tech Debt
SecurityNo findings. Documentation/TestsNo findings. The dynamic suite covers roster completeness, signature synchronization, atomic rollback, fitted-state preservation, and clone behavior ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
diff_diff/_base.py: the sharedBaseEstimatormixin replaces the 25 hand-rolledget_params/set_paramspairs (6 divergent implementation shapes, 12 non-transactional) library-wide - v4-design §9 item 3 / 2(c)-i, front-loaded so the 2(c)-ii renames build on the transactional contract. The exhaustive inventory found 25 defining classes, correcting the design doc's count of 24.get_paramsintrospects the__init__signature (per-class cache;deep=accepted uniformly - previously 4/25) so the parameter surface can never drift from the constructor.set_paramsis transactional via probe re-init: strict unknown-key gate, an overridable_normalize_set_paramshook, thentype(self)(**merged)validates before any mutation and the probe's parameter + declared derived-config attrs are adopted. Fitted state is never touched._PARAM_ATTR_ALIASES(DiD rawvcov_typeat_vcov_type_arg; PreTrendsPowerpower→target_power),_DERIVED_CONFIG_ATTRS(DiD vcov trio;_vcov_type_expliciton CS/SunAbraham/WooldridgeDiD), DiD's 4-line hook for the pinnedrobust=-alone re-derivation, and SyntheticDiD__init__raw-arg storage (deprecatedlambda_reg/zetanormalized toNone; get_params now mirrors the full signature).set_paramsaccepted constructor-rejected values untilfit()now raise atset_params;BaconDecompositionno longer silently ignores unknown keys; unknown-key rejection is uniformly strict;HonestDiDgains rollback,SpilloverDiDbatch atomicity;TROPempty-grid and ContinuousDiD/PreTrendsPower list-normalization edge cases aligned with their constructors;get_paramskey order follows__init__.tests/test_base_estimator.pyon a dynamically discovered roster (every exportedfit-class must mixBaseEstimatorin;LinearRegressionis the one reasoned exclusion); eight lazy-validation test pins flipped to eager expectations;premerge_scan.pyCheck C now fires only for hand-rolled in-bodyget_params, with fixtures both ways.docs/apipages gain:inherited-members:so the relocated methods stay rendered; DEFERRED.md decision rows (DCDH clone-identity won't-fix; scikit-learn stays out of dev deps) and a TODO.md follow-up row; v4-design §7/§9 marked shipped.Methodology references (required if estimator / math changes)
set_paramssplit) is retired because eager transactional validation is now the uniform library-wide contract (REGISTRY Note updated in this diff).Validation
tests/test_base_estimator.py(new, 209 passing checks over the dynamic roster),tests/test_premerge_scan.py(Check C fixtures both ways), lazy-validation pin flips intests/test_staggered.py,tests/test_triple_diff.py,tests/test_two_stage.py,tests/test_imputation.py,tests/test_methodology_imputation.py; message-standardization updates intests/test_had.py,tests/test_continuous_did.py;tests/test_methodology_sdid.pyget_params key-set update. Full param/atomicity suites run green across all 25 estimators (had, rdd, changes_in_changes, estimators_vcov_type, conley_vcov, methodology_synthetic_control, methodology_sdid, wooldridge, lpdid, sun_abraham, stacked_did, spillover, bacon, pretrends, honest_did, efficient_did, chaisemartin_dhaultfoeuille, staggered, staggered_triple_diff, trop, two_stage, imputation, methodology_callaway, methodology_did, estimators); naming guard, v4 matrix, doc-deps integrity, and docs-IA suites green; docs build spot-check confirmsget_params/set_paramsrender on the pages that gained:inherited-members:.Security / privacy